Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DEVEX-179] Add a TF module to make mono-repository setup easier #148

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open

[DEVEX-179] Add a TF module to make mono-repository setup easier #148

wants to merge 45 commits into from

Conversation

Krusty93
Copy link
Contributor

@Krusty93 Krusty93 commented Oct 28, 2024 •
edited
Loading

List of changes

This PR adds a new Terraform module with to make the mono-repository (with single Azure env) setup simpler.

This module includes:

  • an Azure resource group for the entire mono-repository
  • managed identities federated with GitHub for:
    • "infra" pipelines
    • "app" pipelines
    • "opex" pipelines
  • IAM management for:
    • AD teams
      • team admins
      • team developers
      • (eventually) team external members
    • Managed identities mentioned above
  • GitHub runner

If a team needs more resource groups, they can be defined and managed into the resources configuration. However, all roles on that scope must be defined there as well.

Constraints:

Versions constraints are intentionally high.

Azurerm pretends >= v4
Terraform versions should be >= 1.9.x

Requirements:

Before using this module, team must ensure to have at least two AD groups (admins, devs and eventually externals) with different subscription roles

Motivation and context

Two main reasons brought the creation of this module:

  • devs can't manage locks on their own resources and this is a bottleneck. With this module, team admins will be able to do it
  • currently, each dev can add, modify and destroy resources of other teams, but the only reason why a member of a team would do it is a mistake. In fact, this is not a need but only a consequence of the current management. Adopting patterns induced by this module, permissions will shifts to a read-only approach

Moreover, it avoids manual setup for "apps" and "opex" managed identities as the actual module uses "infra" roles as defaults.

This module completes the following setup:

Subscription Team Resource Group Private Endpoints DNS Zone APIM Team KeyVault Resource Group IAM Resource Lock
ID Infra CI Reader √ (Reader) √ (Reader) PagoPA DNS Zone Reader √ (Reader) Secrets, Keys, Certificates X X
ID Infra CD Reader Contributor Network Contributor Private DNS Zone Contributor API Management Service Contributor Secrets, Keys, Certificates Role Based Access Control Administrator User Access Administrator
ID App Reader Contributor √ (Contributor) √ (Contributor) √ (Contributor) X X X
ID Opex Reader Opex Contributor X X X X X X
AD Admins Contributor Owner √ (Contributor) √ (Contributor) √ (Contributor) Secrets, Keys, Certificates X √ (Owner)
AD Developers Reader Contributor X X X Secrets X X
AD Externals Reader √ (Reader) X X X X X X

Why now?

Patterns and needs are clearer now, then making assumptions is easier

Type of changes

  • Add new resources
  • Update configuration to existing resources
  • Remove existing resources

Does this introduce a change to production resources with possible user impact?

  • Yes, users may be impacted applying this change
  • No

Other information

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Oct 28, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 9adf5a4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Krusty93 Krusty93 changed the title [DEVEX-179] Add a TF module for mono-repository setup [DEVEX-179] Add a TF module to make mono-repository setup easier Oct 29, 2024
@Krusty93 Krusty93 marked this pull request as ready for review November 5, 2024 11:28
@Krusty93 Krusty93 requested review from a team as code owners November 5, 2024 11:28
gunzip
gunzip reviewed Nov 7, 2024
View reviewed changes
infra/modules/azure_monorepo_single_env_starter_pack/github_branch_rules.tf
pattern = "main"

required_status_checks {
strict = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any value in having the branch up to date before merging as CI pipelines run over the merge commit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you elaborate a little bit more?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we're talking about monorepository (aka a single, potentially large repository), there is an high possibility that PR's branch is not up to date with main as other people are working on the same code base. I don't see this as an issue because the delta does not affect the validity of CI checks nor "manual" review as changes are on different paths. On the other hand, if changes are on the same files it is likely to have conflicts and PR cannot be merged

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's see what @pagopa/engineering-team-devex thinks about this

gunzip
gunzip reviewed Nov 7, 2024
View reviewed changes
infra/modules/azure_monorepo_single_env_starter_pack/github_repository.tf
advanced_security {
status = "enabled"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add a validation to set required tags (team name / domain)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain it a little more?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean: tags shouldn't ever be an empty map, but contains at least the team / domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which tags? I don't follow you :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the usual ones Owner , Source, ... aren't required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub resources don't have the concept of tags, which is strictly related to Azure

gunzip
gunzip requested changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@gunzip gunzip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor improvements

@Krusty93 Krusty93 force-pushed the DEVEX-179-produrre-un-modulo-terraform-per-migliorare-la-gestione-dei-permessi-rbac-sui-resource-group branch from ccc23f3 to ceee0f6 Compare November 19, 2024 11:43
@Krusty93 Krusty93 force-pushed the DEVEX-179-produrre-un-modulo-terraform-per-migliorare-la-gestione-dei-permessi-rbac-sui-resource-group branch from 1d0fd83 to b12acee Compare January 7, 2025 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gunzip gunzip gunzip requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Krusty93 @gunzip